fix: accept GitHub URLs in settings add project#1110
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughExtracted path/URL detection into a reusable getCodebaseInput(value: string) utility returning { url } or { path }. Replaced duplicated inline detection in Sidebar, ChatPage, and SettingsPage; re-exported the helper from api.ts; added unit tests and docs describing the new Add Project input flow. (46 words) Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/web/src/lib/api.ts`:
- Around line 24-26: The isLocalPath check currently only treats absolute-style
paths as local; update the condition in the isLocalPath assignment so relative
paths (trimmed.startsWith('./') and trimmed.startsWith('../')) and bare names
(no URL scheme) are classified as local. You can do this by either adding checks
for './' and '../' to the existing expression or by inverting the logic: detect
URLs with a URL-scheme regex (e.g. scheme://) and treat anything that does not
match that scheme pattern as local. Modify the isLocalPath calculation (using
trimmed) so the function still returns { path: trimmed } for those relative/bare
inputs and { url: trimmed } only when a proper URL scheme is detected.
In `@packages/web/src/lib/codebase-input.test.ts`:
- Around line 4-28: Add regression tests to getCodebaseInput to cover relative
and UNC local paths: add cases asserting that inputs like './repo', 'repo' (bare
relative), and '\\\\server\\share' (UNC path) are classified as paths (returning
{ path: '...' }). Update the test suite around the getCodebaseInput describe
block by adding new test(...) entries for these inputs so they fail if
local-path classification regresses; reference the getCodebaseInput function in
your changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 53f1224b-556e-4140-91d7-2260896e7284
📒 Files selected for processing (4)
packages/web/src/components/layout/Sidebar.tsxpackages/web/src/lib/api.tspackages/web/src/lib/codebase-input.test.tspackages/web/src/routes/SettingsPage.tsx
|
Thanks for raising #1108 and putting up the initial fix, @LocNguyenSGU — the shared I just pushed Critical fixes
Structural improvements
Test coverage
Docs
Validation
One operational note before merge
Heuristic note worth flagging in the PR body: with this change, ambiguous inputs (e.g., GitHub shorthand Thanks again for the fix. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/web/src/routes/SettingsPage.tsx (1)
277-279: Minor: destructuredallowEnvKeysshadows the component-scope state.The mutation parameter
allowEnvKeyson line 278 shadows theallowEnvKeysstate declared at line 268. Behavior is correct (the mutate arg is used), but given the ESLint zero-tolerance policy this could be flagged byno-shadow/@typescript-eslint/no-shadowif enabled. Consider renaming the mutation parameter (e.g.,allowEnvKeysOverride) for clarity.♻️ Suggested rename
- mutationFn: ({ value, allowEnvKeys }: { value: string; allowEnvKeys?: boolean }) => - addCodebase({ ...getCodebaseInput(value), allowEnvKeys }), + mutationFn: ({ value, allowEnvKeys: allow }: { value: string; allowEnvKeys?: boolean }) => + addCodebase({ ...getCodebaseInput(value), allowEnvKeys: allow }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/routes/SettingsPage.tsx` around lines 277 - 279, The mutation parameter name allowEnvKeys in the addMutation useMutation call shadows the component-scoped allowEnvKeys state; rename the parameter (e.g., allowEnvKeysOverride) in the mutationFn signature and update its usage where calling addCodebase({ ...getCodebaseInput(value), allowEnvKeys }) to use the new name (allowEnvKeysOverride) so the component state and mutation argument are unambiguous; ensure references to addMutation and getCodebaseInput are updated accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/web/src/routes/SettingsPage.tsx`:
- Around line 277-279: The mutation parameter name allowEnvKeys in the
addMutation useMutation call shadows the component-scoped allowEnvKeys state;
rename the parameter (e.g., allowEnvKeysOverride) in the mutationFn signature
and update its usage where calling addCodebase({ ...getCodebaseInput(value),
allowEnvKeys }) to use the new name (allowEnvKeysOverride) so the component
state and mutation argument are unambiguous; ensure references to addMutation
and getCodebaseInput are updated accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d489bee2-c405-4110-b598-2bd28b5a9ba7
📒 Files selected for processing (6)
packages/docs-web/src/content/docs/adapters/web.mdpackages/web/src/lib/api.tspackages/web/src/lib/codebase-input.test.tspackages/web/src/lib/codebase-input.tspackages/web/src/routes/ChatPage.tsxpackages/web/src/routes/SettingsPage.tsx
✅ Files skipped from review due to trivial changes (1)
- packages/web/src/lib/codebase-input.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/web/src/lib/api.ts
…oints
Settings → Projects Add Project only submitted { path }, so GitHub URLs
entered there failed even though the API and the Sidebar Add Project
already accepted them. Closes coleam00#1108.
Changes:
- Add packages/web/src/lib/codebase-input.ts: shared getCodebaseInput()
helper returning a discriminated { path } | { url } union (re-exported
from api.ts for convenience).
- Use the helper from all three Add Project entry points: Sidebar,
Settings, and ChatPage. Removes three divergent inline heuristics.
- SettingsPage: rename addPath → addValue (state now holds either URL
or local path) and update placeholder text.
- Tests: cover https://, git@ shorthand, ssh://, git://, whitespace,
unix/relative/home/Windows/UNC paths.
- Docs: document the unified Add Project entry point in adapters/web.md.
Heuristic flips from "assume URL unless explicitly local" to "assume
local unless explicitly remote" — only inputs starting with https?://,
ssh://, git@, or git:// are sent as { url }; everything else is sent
as { path }. The server already resolves tilde/relative paths.
Co-authored-by: Nguyen Huu Loc <lockbkbang@gmail.com>
leex279
force-pushed
the
fix/issue-1108-settings-add-project-url-support
branch
from
433a835 to
9dd57b2
Compare
|
Rebased onto current Because the conflicts touched logic that no longer exists on The end state is unchanged from what was reviewed:
Validation re-run after rebase:
|
|
The failing check ( Filed as #1265 with reproduction steps, root cause, and three suggested fixes. The same step fails on This PR remains ready to merge once #1265 is resolved (or the Windows check is treated as a known-failing pre-existing issue and not a blocker). |
…tings-add-project-url-support fix: accept GitHub URLs in settings add project
2 participants
Summary
Make the Settings page Add Project flow accept GitHub repository URLs the same way as the sidebar.
Reproduction
The server API supports either
{ url }or{ path }, and the sidebar already classifies input accordingly.SettingsPage.tsxalways submitted{ path }, so entering a GitHub repository URL there behaved differently from the sidebar.Patch Summary
getCodebaseInput()helper for path-vs-URL classificationRisks
Low. The change only affects Add Project input classification and keeps existing local-path handling intact.
Validation
cd packages/web && bun test src/lib/codebase-input.test.tsSummary by CodeRabbit
New Features
Refactor
Tests
Documentation